Scheduled Txs: Updates limits and add removal limit #546
Conversation
| } | ||
|
|
||
| if numRemoved >= self.config.collectionTransactionsLimit { | ||
| break |
There was a problem hiding this comment.
add an event to be emitted here ItterationLimitReached
There was a problem hiding this comment.
this should be seperate value from collection transaction limit, just so we don't confuse the meaning of those, also they might not be same. Let's create another variable called collectionItterationLimit or something like that, and make sure it is well documented in comments, something like "this defines the limit of itterations on the transaction array in each process call"
There was a problem hiding this comment.
Can we just make that a multiple of the collectionTransactionLimit? like x2 or x1.5 something? I'd prefer to avoid adding new fields to the config unless we have to and this value is already related to the collectionTransactionLimit anyway, so I think it makes sense to make it a multiple.
There was a problem hiding this comment.
I wouldn't because then we can't control them individually, so we would potentially want to slow down remove-al but not slow down execution, to throttle the issue we observed.
There was a problem hiding this comment.
In order to add a new field to the struct, we have to add a whole new struct definition, and upgrades do not allow us to remove struct definitions, so we would have two struct definitions in the contract. I think we should hard code this for now, and then we can change it with a contract upgrade anyway, then in the future, when we decide to add other fields to the config, we can add this also. I just don't want superfluous config struct definitions stacking up.
Another option is adding a function to the struct and storing the value in the account storage, then just getting that value in the struct. It wouldn't be accessible if the struct is returned to sdk code, but it would still be accessible in Cadence
joshuahannan
force-pushed
the
josh/test-process
branch
from
eb6113c to
ef58ab0
Compare
joshuahannan
changed the title
process() more efficient
|
|
||
| numRemoved = numRemoved + 1 | ||
|
|
||
| let removalLimit = self.config.collectionTransactionsLimit + (self.config.collectionTransactionsLimit / 2) |
There was a problem hiding this comment.
This worries me because if we want to limit the removal to low (if removal caused same issue as it did in stress test) we would have to also limit execution, which would grow the backlog for execution and in turn backlog for removal. I know upgrade limitation justify this decision, but at least lets add a todo or some comment about refactoring this in future.
There was a problem hiding this comment.
I just updated it in my last commit to store the value in account storage and access it with a function. This will make it so it is configurable now separate from the transaction limit.
| } | ||
|
|
||
| access(all) view fun getTxRemovalLimit(): UInt { | ||
| return FlowTransactionScheduler.account.storage.copy<UInt>(from: /storage/txRemovalLimit) |
There was a problem hiding this comment.
can we cache this once it's loaded
2 participants
This PR focuses on optimizing the Scheduled Transactions feature.
The key changes involve:
Limit Updates: Lowers cumulative effort limits and all priority effort limits by half
Removal limit: Adds a limit to the number of transactions that can be removed during
removeExecutedTransactions()so it doesn't hit the gas limit